Skip to content

op-node: fix sync heads starting point [bedrock]#3320

Closed
protolambda wants to merge 2 commits intodevelopfrom
safe-head-fix
Closed

op-node: fix sync heads starting point [bedrock]#3320
protolambda wants to merge 2 commits intodevelopfrom
safe-head-fix

Conversation

@protolambda
Copy link
Contributor

@protolambda protolambda commented Aug 26, 2022

Description

This PR fixes the way we find the L2 block that we start syncing on top of.

Off by one fix in safe head

As I was working on tests for the finalization, I found that the safe-head sync algorithm has an off-by-one error:

  • We reset back a full sequence window, but stop at the L2 block that has a L1 origin exactly n blocks from the common point, inclusive. This means that the safe head we're left with after completing the reset, is the one block with sequence nr 0, within the scope of the window. Which can point to an older L1 origin, but where the inclusion is within the window.
  • Meanwhile, if this exact batch wasn't included anymore after a deep reorg, or e.g. if the L1 chain we reorged to has no batches at all for a full window, then an alternative (or empty L2 block) would be there.
    We need to reset one L2 block deeper, to force all previous safe L1 blocks (only those that we can't be sure of with inclusion even though their L1 origin is canonical) to be reconciled against L1, to ensure they are still included on the new canonical L1 chain.

Safe block RPC label usage

Currently we're also just doing the above starting at the unsafe head instead of safe head, meaning the off-by-one error can be higher if we happen to have a long unsafe chain that still references canonical L1 data but isn't actually submitted on-chain.
This is a long-due fix for a workaround that couldn't be fixed due to the lack of support for the safe block label in the geth RPC.

Finalized block check

The initialization of the finalized info is now part of the sync start algorithm, since we need to check if we don't revert it accidentally.

Metadata

Fix ENG-2671

@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2022

⚠️ No Changeset found

Latest commit: a4449c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2022

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Aug 26, 2022
Base automatically changed from finality-data to develop August 26, 2022 23:47
@mergify mergify bot removed the conflict label Aug 27, 2022
@protolambda protolambda marked this pull request as ready for review August 27, 2022 02:47
…correctly, and fix off-by-1 in epoch window boundary
@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2022

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Sep 9, 2022
@protolambda protolambda mentioned this pull request Sep 9, 2022
7 tasks
@mslipper
Copy link
Contributor

This has been replaced.

@mslipper mslipper closed this Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants